Cleanup InferParameterBindingInfoConvention #8665
Conversation
4447cd1
to
22dc79c
Compare
@pranavkm I think I should hold off reviewing 'til you split this in two. LMK if I need to review this up front. |
eb248c4
to
9bb5480
Compare
🆙 📅 |
* Infer BindingSource for collection parameters as Body. Fixes #8536 * Introduce a compat switch to keep 2.1.x LTS behavior for collection parameters * Do not infer BinderModelName in InferParameterBindingInfoConvention
9bb5480
to
d98c27f
Compare
@pranavkm the commit description and the title are way wonky. This is about a new option ( |
@@ -151,6 +154,39 @@ public bool SuppressUseValidationProblemDetailsForInvalidModelStateResponses | |||
set => _suppressUseValidationProblemDetailsForInvalidModelStateResponses.Value = value; | |||
} | |||
|
|||
/// <summary> | |||
/// Gets or sets a value that determines if <see cref="BindingInfo.BindingSource"/> for collection types | |||
/// (<see cref="ModelMetadata.IsCollectionType"/>) is inferred as <see cref="BindingSource.Query"/>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mention what binding source is chosen otherwise.
/// guidance and examples of setting the application's compatibility version. | ||
/// </para> | ||
/// <para> | ||
/// Configuring the desired value of the compatibility switch by calling this property's setter will take |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor but s/will take/takes/
@@ -66,8 +67,6 @@ internal class ApiBehaviorApplicationModelProvider : IApplicationModelProvider | |||
|
|||
public List<IActionModelConvention> ActionModelConventions { get; } | |||
|
|||
public List<IControllerModelConvention> ControllerModelConventions { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see we don't need IControllerModelConvention
s anymore. But, what about our customers? Suggest leaving the property and the application of any items in the list. That is, limit change to initializing it as empty.
If this is a removal we've already decided on, remove the IControllerModelConvention
interface as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type is internal, so this does not affect users.
/// The goal of this covention is to make intuitive and easy to document <see cref="BindingSource"/> inferences. The rules are: | ||
/// <list type="number"> | ||
/// <item>A previously specified <see cref="BindingInfo.BindingSource" /> is never overwritten.</item> | ||
/// <item>A complex type parameter (<see cref="ModelMetadata.IsComplexType"/>) <see cref="BindingSource.Body"/> is assigned.</item> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please copy over my doc comment improvement for ModelMetadata.IsComplexType
.
Also s/<see cref="BindingSource.Body"/>
is assigned/is assigned <see cref="BindingSource.Body"/>
/. Same for the next two items.
/// Gets or sets a value that determines if model binding sources are inferred for action parameters on controllers is suppressed. | ||
/// </summary> | ||
public bool SuppressInferBindingSourcesForParameters { get; set; } | ||
internal bool AllowInferringBindingSourceForCollectionTypesAsFromQuery { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not public
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This convention type is new to 2.2, so users wouldn't need a compat flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🆙 📅
@@ -66,8 +67,6 @@ internal class ApiBehaviorApplicationModelProvider : IApplicationModelProvider | |||
|
|||
public List<IActionModelConvention> ActionModelConventions { get; } | |||
|
|||
public List<IControllerModelConvention> ControllerModelConventions { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type is internal, so this does not affect users.
/// Gets or sets a value that determines if model binding sources are inferred for action parameters on controllers is suppressed. | ||
/// </summary> | ||
public bool SuppressInferBindingSourcesForParameters { get; set; } | ||
internal bool AllowInferringBindingSourceForCollectionTypesAsFromQuery { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This convention type is new to 2.2, so users wouldn't need a compat flag.
Fixes #8657